Michelle & Sigrid - Edges - Video Store API#11
Michelle & Sigrid - Edges - Video Store API#11sdbenezra wants to merge 64 commits intoAda-C10:masterfrom
Conversation
Set up controllers
Model setup
Customer testing
prelim model (validations+relations) + controller tests for movie
…for returned customer data. Complete customer controller testing.
Add new movies_checked_out column for customers. Add json formatting …
set up movies routes + controller for index and show; create still un…
…dded available_inventory column to movies
Mc movies controller
got Movies#create working
Rework rentals checkin method to use post. Tests passing for all but inventor…
Mc testing movies
…tory methods. Fix reference to customer in customer test file.
…d checkin methods.
Rework rentals controller and test to account for changed movie inven…
added null:false to movies inventory column
Mc testing movies
Video StoreWhat We're Looking For
Great work overall you two! In general, great work on creating this API. The code could be tidied up in some parts, but it's overall good. I like your model and controller tests a lot, too. I'm adding a few comments, but overall: well done PS: This project lists Ruby 2.4.1 as a dependency even though we've been working in 2.5.1. Just curious? |
|
|
||
| def jsonify(customer_data) | ||
| return customer_data.as_json( only: [:id, :name, :registered_at, :address, :city, :state, :postal_code, :phone, :movies_checked_out_count]) | ||
| end |
|
|
||
| if movie | ||
| avail = movie.calculate_available_inventory | ||
| result = movie.save_available_inventory(avail) |
There was a problem hiding this comment.
You end up assigning the result of movie.save_available_inventory(avail) into the variable result, but you don't use result anywhere... you probably don't need to assign this variable to anything
|
|
||
| result = movie.save | ||
| if result | ||
| movie_id = movie.id |
There was a problem hiding this comment.
Same as above: No need to make the movie_id variable here
| if rental.save | ||
| inventory = movie.calculate_available_inventory() | ||
| movie.save_available_inventory(inventory) | ||
| customer[:movies_checked_out_count] += 1 |
There was a problem hiding this comment.
Could you make a method that increments the customer's checked out count field instead of operating on it directly here?
| validates :movie_id, presence: true | ||
|
|
||
| def checked_out?() | ||
| return self.checkedout == true |
There was a problem hiding this comment.
because of the nature of this conditional, you could probably just return self.checkedout
| rental = Rental.new(rental_params) | ||
| if rental.save | ||
| inventory = movie.calculate_available_inventory() | ||
| movie.save_available_inventory(inventory) |
There was a problem hiding this comment.
The two above lines are tightly coupled/repeated a lot. Could there be a method on Movie so an instance of Movie can update its available inventory itself?
| body = JSON.parse(response.body) | ||
| expect(body).must_be_kind_of expected_type | ||
| return body | ||
| end |
There was a problem hiding this comment.
Nice! This looks really slick! I really like this default argument!
| body = JSON.parse(response.body) | ||
| expect(body).must_be_kind_of expected_type | ||
| return body | ||
| end |
There was a problem hiding this comment.
Since this helper method is in multiple files, you could probably bring it out into the test_helper file
| expect(body).must_include "errors" | ||
|
|
||
| must_respond_with :success | ||
| end |
3 participants
Video Store API
Congratulations! You're submitting your assignment!
If you didn't get to the functionality the question is asking about, reply with what you would have done if you had completed it.
Comprehension Questions
POST /rentals/check-inendpoint? What does the time complexity depend on? Explain your reasoning.